Update to Work with Eigen 5.0.0 #536
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates FindEigen3.cmake to support both Eigen 5.x and 3.x version detection, as the version header location changed in Eigen 5.0.0. However, the PR also includes several unrelated changes in test files.
Changes:
- Updated FindEigen3.cmake to check for version headers in both Eigen 5.x location (Eigen/Version) and Eigen 3.x location (Eigen/src/Core/util/Macros.h)
- Refactored constructors in tests/sparse_tile.h to use IIFE pattern for running assertions before member initialization
- Modified test in tests/expressions_impl.h to use a temporary variable instead of reusing the same array
- Fixed assertions in src/TiledArray/tensor/tensor_interface.h to check member variables instead of parameters
- Changed spacing around archive operators in tests/sparse_tile.h
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmake/modules/FindEigen3.cmake | Adds support for Eigen 5.x version header location with fallback to Eigen 3.x location |
| tests/sparse_tile.h | Refactors constructors to use IIFE pattern and modifies archive operator spacing |
| tests/expressions_impl.h | Changes test to use temporary variable for block operations |
| src/TiledArray/tensor/tensor_interface.h | Fixes assertions to check member variables after initialization |
Comments suppressed due to low confidence (1)
cmake/modules/FindEigen3.cmake:54
- After reading the version header file, if the regex matches fail (lines 47-52), the version variables (EIGEN3_WORLD_VERSION, EIGEN3_MAJOR_VERSION, EIGEN3_MINOR_VERSION) will be empty strings. This could lead to an incorrect EIGEN3_VERSION being set on line 54 (e.g., "..") and potentially unexpected behavior in version comparison on line 55. Consider adding validation after the regex matches to ensure the version numbers were successfully extracted, or set EIGEN3_VERSION_OK to FALSE if any of the version components are empty.
string(REGEX MATCH "define[ \t]+EIGEN_WORLD_VERSION[ \t]+([0-9]+)" _eigen3_world_version_match "${_eigen3_version_header}")
set(EIGEN3_WORLD_VERSION "${CMAKE_MATCH_1}")
string(REGEX MATCH "define[ \t]+EIGEN_MAJOR_VERSION[ \t]+([0-9]+)" _eigen3_major_version_match "${_eigen3_version_header}")
set(EIGEN3_MAJOR_VERSION "${CMAKE_MATCH_1}")
string(REGEX MATCH "define[ \t]+EIGEN_MINOR_VERSION[ \t]+([0-9]+)" _eigen3_minor_version_match "${_eigen3_version_header}")
set(EIGEN3_MINOR_VERSION "${CMAKE_MATCH_1}")
set(EIGEN3_VERSION ${EIGEN3_WORLD_VERSION}.${EIGEN3_MAJOR_VERSION}.${EIGEN3_MINOR_VERSION})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
EricaCMitchell
force-pushed
the
emitchell/upgrade/eigen-5.x.x
branch
from
486a095 to
ad4bfd1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Eigen3 5.x.x makes a change in its allocation if For an empty |
Description
Eigen 5.0.0 changes where version is located. Needs to find version header for Eigen 5.x.x and Eigen 3.x.x. Same change as Libint PR 395.
Details
EIGEN_HAS_CXX17, which is set ifEIGEN_MAJOR_VERSION> 4EigenSparseTileTensorInterface